stat files before serveIndex, also output date, size, and type #74

coolaj86 · 2018-08-13T01:01:36Z

As discussed in #73 this PR moves the stating of files into serveIndex so that serveIndex.plain, serveIndex.json, and serveIndex.html consistently output the same data

~~This does not yet include the updates to the tests.~~ Tests are passing.

Also addresses #39

…d type

dougwilson · 2018-08-13T04:23:14Z

index.js

+    var type = accept.type(mediaTypes);
+
+    // not acceptable
+    if (!type) return next(createError(406));


This seems to break using this module above your other handlers. This will block all request that doesn't have the right Accept header even if this module is just going to do nothing because the folder doesn't exist.

Makes sense.

I originally moved it up so that it would be checked before all of the fs.stat()s. I moved it just after the directory stat and before the fs.readdir.

dougwilson · 2018-08-13T04:31:13Z

index.js

-  send(res, 'text/plain', (files.join('\n') + '\n'))
+serveIndex.plain = function _plain(req, res, dir, files) {
+  var directory = {
+    name: dir.name.replace(/\/$/, '\/')


Does this replace actually do anything? I can't figure out what it does, can you ellaborate?

change to dir.name.replace(/\/?$/, '/')
/ => /
demo => demo/

dougwilson · 2018-08-13T04:32:33Z

index.js

+ */
+
+var SIZES = [ 'B', 'K', 'M', 'G', 'T', 'P', 'E' ]
+function hsize(size) {


Use a module here instead of adding this code to the module. The bytes module is one I know, but you're welcome to use a different one if there is one you like better.

That makes me sad. There's so many packages in node that do next to nothing and somehow end up including 100's of megabytes of download. The bytes package is WAY overkill for something this simple.

That said, I'll make the change.

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

It can be a diff package if there is a better one like I said. The other alternative is that this would need tests to cover the conversations (for instance I deleted the "E" from the list and no tests seemed to catch that -- I also changed 1024 to 999 and nothing failed) and it creates more things I have to maintain in the long term, which is harder when it doesn't have tests.

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

Ok, but why would it be any different that this module itself cannot be compromised?

dougwilson · 2018-08-13T05:08:01Z

Just as a general comment, this pr is doing many changes all at once. That's fine to review up front, but it'll need to be split into discrete changes prior to eventually landing. The standard practice is that each line in the history file describes a discrete change which in turn is one commit. You can alternatively keep each discrete change organized in their own commits in the single pr and it can be rebase merged instead of the normal squash merged.

This is just a note now so it's not a surprise later when it because difficult to split apart.

coolaj86 · 2018-08-13T05:17:41Z

In my mind all of the changes are tied together. Moving where the stat() is done changes the API significantly. I don't believe I've made any changes in this string of commits that are not directly related to that.

Do you just want me to rebase this with a single comment like "expose fs.stat() to plain, json, and html templates"?

Or do you want this broken down in some other way?

dougwilson · 2018-08-13T05:18:59Z

The showing the file size in human format instead of bytes is clearly unrelated to me, if that helps as an example.

coolaj86 · 2018-08-13T05:22:40Z

@dougwilson Before it didn't show size at all. Now that it shows size, it must show it in some way.

Before when it didn't show bytes, the files were aligned and easy to read.

To show it in raw bytes would break the formatting, which would be a bigger change (in my mind) than the alternative.

In my mind I've done the absolute minimum to expose the data in the most standard way with the most minimal change to the overall experience of anyone using it presently (despite the breaking change).

Can you give me a breakdown of what you'd like and I'll redo it whichever way?

dougwilson · 2018-08-13T05:22:54Z

Though I am on my phone so hard to take in the entire changeset, it seems to break apart into separate commits like the following to me:

Provide full file into to formatter functions
Change html file size to human format
Display addition file information in json format
Display additional file information in text format

I hope that help :)

coolaj86 · 2018-08-13T05:23:53Z

html file size was not changed

coolaj86 · 2018-08-13T05:24:43Z

Got it. 1 big step forward, 3 steps back, then 3 individual steps forward.

dougwilson · 2018-08-13T05:25:38Z

Sorry, as I said I'm on my phone and just guessing to help provide an example :) I just assume you made them a display the same way irt human sizes, my bad.

dougwilson · 2018-08-13T05:27:33Z

I'm happy to help guide you with large changes to match the existing practices, but you're bot asking how to do anything first, just doing it. If you don't want to run i to issues, perhaps we should stop and get together and discuss so we're on the same page? Not sure if you know another way. I'm trying to be helpful but comments like that come off kind of rude.

coolaj86 · 2018-08-13T06:32:05Z

Well, I made another two PRs, much smaller. I'll probably open a few more, but feel free to ignore them until you're happy with the first two.

I thought I had asked the questions that were important to ask in the previous issue. I prefer to just do stuff and talk about it later - because then there's something concrete and tangible to talk about. I feel like code communicates more effectively than English does.

The only pet peeve I have is when things get to a point of bickering about how to name variables. I'm happy to do 95% of the work, but when it comes to itsy bitsy details of personal preference I expect maintainers to click the edit button themselves at that point - I'm a human, not a Siri for dictating and retyping codestyle endlessly.

That said, seeing as how this codebase is fairly inconsistent and has multiple authors using the same variable names in different contexts to mean different things, I don't expect that to be a problem here.

dougwilson · 2018-08-13T11:54:03Z

I'm not sure if you are confusing me for someone else here. I have not made any kind of comment along the lines of what the variables should be named as.

coolaj86 · 2018-08-13T14:24:46Z

Rereading my comment...

You had said that I’m not asking, I’m just doing.

I was saying that it’s easier for me to just do and then talk (because then the communication is 100%), I don’t mind reworking something - except if it comes to the point of dictating down to the names of variables. I wasn’t accusing you of doing that.

However, it is quite common on GitHub for people to spend more time critiquing minutia than it would have taken them to simply make the change.

Sorry for the confusion.

coolaj86 · 2018-08-20T17:52:23Z

@dougwilson Just wanted to make sure you saw my comment above: I was not accusing you.

I was saying that I don't mind doing a number of PRs and getting feedback - because I prefer to communicate in code rather than English for implementation details - as long as it doesn't get down to mundane minutia like variable naming.

I made a flurry of PRs and was expecting to have some more back and forth so that you can see exactly what my intent is, without any ambiguity, and that I can get your feedback on that.

dougwilson · 2018-08-23T12:42:37Z

Yes, I did read your comment :) I have just been away (my github activity reflects that as well). I will be back soon and will take a look, though there was a security issue reported while I was away, which may take some of my attention at first.

coolaj86 · 2018-12-26T21:52:04Z

@dougwilson I'd still be interested in getting some of these commits through.

My ultimate goal is to make it easy to have arbitrary user templates (using a variation of the existing template as a default).

expressjs#73 stat files before serveIndex, also output date, size, an…

8c4ef23

…d type

coolaj86 mentioned this pull request Aug 13, 2018

option to download files rather than view #73

Open

AJ ONeal added 2 commits August 12, 2018 20:32

use dir as string, not object

bbfd384

expressjs#73 update tests for first PR

af9d2be

dougwilson added enhancement pr labels Aug 13, 2018

dougwilson reviewed Aug 13, 2018

View reviewed changes

AJ ONeal added 2 commits August 12, 2018 22:44

convert from fs.stat to vanilla json before response templates

79d5572

add a single trailing slash to directory name

cb76221

coolaj86 force-pushed the stat-first branch from 2f07272 to cb76221 Compare August 13, 2018 04:53

AJ ONeal added 2 commits August 12, 2018 23:02

use bytes package instead of inline sizer

e9b98d2

check Accept header after directory status

23ba584

coolaj86 changed the title ~~#73 stat files before serveIndex, also output date, size, and type~~ stat files before serveIndex, also output date, size, and type Aug 13, 2018

coolaj86 mentioned this pull request Aug 13, 2018

How to handle fs.stat error on a file? #76

Open

coolaj86 mentioned this pull request Aug 13, 2018

expose fs.stat of root and files to all template functions #79

Open

coolaj86 closed this Aug 13, 2018

coolaj86 mentioned this pull request Aug 13, 2018

allow returning file details in JSON #39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stat files before serveIndex, also output date, size, and type #74

stat files before serveIndex, also output date, size, and type #74

coolaj86 commented Aug 13, 2018 •

edited

Loading

dougwilson Aug 13, 2018

coolaj86 Aug 13, 2018

dougwilson Aug 13, 2018

coolaj86 Aug 13, 2018

dougwilson Aug 13, 2018

coolaj86 Aug 13, 2018

coolaj86 Aug 13, 2018

dougwilson Aug 13, 2018

dougwilson Aug 13, 2018

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018 •

edited

Loading

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

dougwilson commented Aug 13, 2018

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018 •

edited

Loading

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

coolaj86 commented Aug 20, 2018

dougwilson commented Aug 23, 2018

coolaj86 commented Dec 26, 2018

stat files before serveIndex, also output date, size, and type #74

stat files before serveIndex, also output date, size, and type #74

Conversation

coolaj86 commented Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018 • edited Loading

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

dougwilson commented Aug 13, 2018

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018 • edited Loading

dougwilson commented Aug 13, 2018

coolaj86 commented Aug 13, 2018

coolaj86 commented Aug 20, 2018

dougwilson commented Aug 23, 2018

coolaj86 commented Dec 26, 2018

coolaj86 commented Aug 13, 2018 •

edited

Loading

coolaj86 commented Aug 13, 2018 •

edited

Loading

coolaj86 commented Aug 13, 2018 •

edited

Loading